Fix TFMs, exception args, stray semicolons, XML doc, and AGENTS.md from review feedback#45
Fix TFMs, exception args, stray semicolons, XML doc, and AGENTS.md from review feedback#45
Conversation
…s, XML doc, AGENTS.md Co-authored-by: mrdav30 <11547347+mrdav30@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback by correcting .NET target framework monikers, improving exception construction, cleaning up no-op statements, tightening XML docs, and updating contributor/agent guidance to reflect current build/test and serialization practices.
Changes:
- Fix invalid TFMs (
net8→net8.0) and remove obsolete net48 reference-assemblies packages from both library and test projects. - Correct
ArgumentOutOfRangeExceptionusage inFixedQuaternionto avoid populatingParamNamewith message text. - Remove stray no-op semicolons after
switchblocks, and improve XML documentation in bounds types; updateAGENTS.mdworkflow/serialization notes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/FixedMathSharp.Tests/FixedMathSharp.Tests.csproj |
Updates test TFM to net8.0 and removes net48 reference assemblies package. |
src/FixedMathSharp/FixedMathSharp.csproj |
Updates library TFMs (net8.0) and removes conditional net48 reference assemblies package. |
src/FixedMathSharp/Numerics/FixedQuaternion.cs |
Fixes exception constructor usage for angle validation paths. |
src/FixedMathSharp/Bounds/BoundingSphere.cs |
Removes stray no-op semicolon after switch. |
src/FixedMathSharp/Bounds/BoundingBox.cs |
Fills Version XML summary and removes stray no-op semicolon after switch. |
src/FixedMathSharp/Bounds/BoundingArea.cs |
Removes stray no-op semicolon after switch. |
AGENTS.md |
Updates build/test and serialization guidance to current state. |
Comments suppressed due to low confidence (1)
AGENTS.md:18
- AGENTS.md mentions an "exact matrix of OS/TFM combinations" in
.github/workflows/dotnet.yml, but the workflow matrix currently varies only by OS and does not enumerate TFMs. Consider rewording this to avoid implying a TFM matrix (or update the workflow if a TFM matrix is intended).
- CI detail from `.github/workflows/dotnet.yml`:
- Linux and Windows both run `dotnet test` against the supported TFMs (with `net8.0` as the primary test target).
- Refer to the workflow file for the exact matrix of OS/TFM combinations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Check if the angles are in a valid range (-pi, pi) | ||
| if (pitch < -FixedMath.PI || pitch > FixedMath.PI || | ||
| yaw < -FixedMath.PI || yaw > FixedMath.PI || | ||
| roll < -FixedMath.PI || roll > FixedMath.PI) | ||
| { | ||
| throw new ArgumentOutOfRangeException($"Euler angles must be in the range ({-FixedMath.PI}, {FixedMath.PI}), but were ({pitch}, {yaw}, {roll})"); | ||
| throw new ArgumentOutOfRangeException(nameof(pitch), $"Euler angles must be in the range ({-FixedMath.PI}, {FixedMath.PI}), but were ({pitch}, {yaw}, {roll})"); | ||
| } |
There was a problem hiding this comment.
FromEulerAngles validates pitch, yaw, and roll together, but the thrown ArgumentOutOfRangeException always uses paramName: nameof(pitch). If yaw or roll is out of range, ParamName will be misleading for callers. Consider splitting the checks and throwing with the specific offending parameter name, or use ArgumentException (or paramName: null) when the range constraint applies to multiple parameters at once.
Addresses all automated review comments from PR #44 covering invalid TFMs, misused exception constructors, no-op statements, missing docs, and stale agent instructions.
Changes
Project files
net8→net8.0in bothFixedMathSharp.csprojandFixedMathSharp.Tests.csproj(invalid TFM caused silent restore/build failures)Microsoft.NETFramework.ReferenceAssemblies.net48from both projects (no net48 target remains)FixedQuaternion.csFixed
ArgumentOutOfRangeExceptionconstructors that were passing the interpolated message asparamName, resulting in a blank default message and a pollutedParamName:Bounds types
;afterswitchstatements inBoundingBox.cs,BoundingSphere.cs, andBoundingArea.cs<summary>forBoundingBox.VersionAGENTS.mdUpdated build/test workflow, CI description, and serialization notes to reflect the current state:
net8.0primary TFM,dotnet teston both Linux and Windows, and MemoryPack replacing MessagePack/BinaryFormatter.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.